-
Notifications
You must be signed in to change notification settings - Fork 1
*.py files migrated from gardenlinux workflows #179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*.py files migrated from gardenlinux workflows #179
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
==========================================
- Coverage 92.95% 92.43% -0.53%
==========================================
Files 28 30 +2
Lines 1306 1797 +491
==========================================
+ Hits 1214 1661 +447
- Misses 92 136 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| description: "Generated GitHub workflow flavors matrix" | ||
| version: | ||
| description: GardenLinux Python library version | ||
| default: "0.10.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the version here. Don't see an benefit with parametrizing it here.
pyproject.toml
Outdated
| gl-flavors-parse = "gardenlinux.flavors.__main__:main" | ||
| gl-oci = "gardenlinux.oci.__main__:main" | ||
| gl-s3 = "gardenlinux.s3.__main__:main" | ||
| gl-gh = "gardenlinux.github.__main__:main" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last review request is still valid.
| @@ -0,0 +1,3 @@ | |||
| from .__main__ import create_github_release_notes | |||
|
|
|||
| __all__ = ["create_github_release_notes"] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO there's no reason to export this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion calling gardenlinux.github.create_github_release_notes looks much better than gardenlinux.github.main.create_github_release_notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I do agree in general you won't call create_github_release_notes() from any other source than the main entrypoint.
src/gardenlinux/github/__main__.py
Outdated
|
|
||
| s3_artifacts._bucket.download_file( | ||
| release_object.key, artifacts_dir.joinpath(f"{cname}.s3_metadata.yaml") | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still would like to see s3_artifacts.bucket here and everywhere else where used. Reasoning given last time.
src/gardenlinux/github/__main__.py
Outdated
| def download_all_metadata_files(version, commitish): | ||
| repo = Repo(".") | ||
| commit = repo.commit(commitish) | ||
| flavors_data = commit.tree["flavors.yaml"].data_stream.read().decode("utf-8") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure if flavors.yaml should be "checked" out here.
As you are touching a local Git repository here I'm not sure if it is useful to reset the checked out commit each time this function is called. You may just access "whatever"
flavors.yamlis located here instead. Please be aware thatFlavorsParserwill use thefeaturesdirectory that is located here anyway without ensuring that the commitish matches.
src/gardenlinux/github/__main__.py
Outdated
| commitish_short = commitish[:8] | ||
|
|
||
| for flavor in flavors: | ||
| cname = CName(flavor[1], flavor[0], "{0}-{1}".format(version, commitish_short)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same request as last time:
You may use the
commit_idparameter recently added here.
src/gardenlinux/github/__main__.py
Outdated
| def release_notes_compare_package_versions_section(gardenlinux_version, package_list): | ||
| output = "" | ||
| version_components = gardenlinux_version.split(".") | ||
| # Assumes we always have version numbers like 1443.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be aware we'll introduce semantic versioning soon. This logic should use a
semverPython package if suitable and consider the patch version to be zero if only one dot is found in the string.
|
@vivus-ignis please cherry-pick the changes from gardenlinux/gardenlinux#3485 |
Done. |
|
Hate to be the party pooper here, but this PR is far too big. There are many different type changes that this PR contains that can be split up easier. I get that this is a refactor, but we can split it into more mindful pieces. Please split this PR into several small PRs. For instance, test data as a standalone PR. Then there is We change the exports from the Why do we add a binary file to this PR? Can we skip this, or can we generate it? |
|
This is not relevant anymore as the changes were integrated with the code base by a series of separate smaller PRs. |
As agreed with @Akendo, splitting this PR into smaller PRs.
What this PR does / why we need it: This consolidates python CI code from the main gardenlinux repository into the existing python module
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Release creation code was tested on a gardenlinux repo fork: https://github.com/vivus-ignis/gardenlinux-dev/releases
As the bulk of the code was extracted from the gardenlinux/gardenlinux repository, this PR does not show what was changed there. Therefore attaching a diff here:
Diff
Release note: